Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make cftime.datetime - cftime.datetime -> timedelta microsecond-exact #178

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Make cftime.datetime - cftime.datetime -> timedelta microsecond-exact #178

merged 4 commits into from
Jun 23, 2020

Conversation

spencerkclark
Copy link
Collaborator

@spencerkclark spencerkclark commented Jun 21, 2020 •
edited
Loading

Currently subtracting a date from another to produce a timedelta is not microsecond-exact:

In [1]: import cftime

In [2]: cftime.DatetimeNoLeap(2000, 1, 2, 0, 0, 0, 5) - cftime.DatetimeNoLeap(2000, 1, 2)
Out[2]: datetime.timedelta(microseconds=8)

I think this is actually pretty straightforward to fix using some code that already exists in cftime. I'm drawing inspiration here from the way this kind of operation is done in Python's datetime object. As I understand it, we can simply use _IntJulianDayFromDate as something essentially equivalent to datetime.datetime.toordinal, and everything else should basically be the same.

The test confirms the example above is fixed for all calendars, and all other existing tests of timedelta arithmetic pass. @jswhit I think this should enable the exact version of date2num you described in #171 (comment). I could try to add that here or in another PR. As you note, it would be awesome to be able to round-trip dates without loss of precision.

xref: #109 (this would make the function described there no longer necessary)

@jswhit
Copy link
Collaborator

jswhit commented Jun 22, 2020

Wow - very simple and elegant solution! I can merge once you resolve the conflicts.

@spencerkclark
Copy link
Collaborator Author

Thanks @jswhit -- indeed I was pleasantly surprised to find that I could reuse the _IntJulianDayFromDate function, which was already so nicely implemented for the original version of the calculation!

Merge conflicts have been resolved; I think things should be ready to go.

@jswhit jswhit merged commit fa1d226 into Unidata:master Jun 23, 2020
@jswhit
Copy link
Collaborator

jswhit commented Jun 23, 2020

Merging now. I will wait a week or so and then push another release.

@spencerkclark spencerkclark deleted the exact-date2num branch June 23, 2020 14:58
@jswhit
Copy link
Collaborator

jswhit commented Jun 23, 2020 •
edited
Loading

Oops - we forgot the Changelog entry. @spencerkclark - can you create another PR with an updated Changelog for this and PR #176?

@jswhit
Copy link
Collaborator

jswhit commented Jun 23, 2020

Also, it would be good to run the xarray test suite to make sure this has not broken anything before a new release

@spencerkclark spencerkclark mentioned this pull request Jun 24, 2020
Merged
@spencerkclark
Copy link
Collaborator Author

Oops - we forgot the Changelog entry.

Ah yes, thanks for reminding me -- see #179.

Also, it would be good to run the xarray test suite to make sure this has not broken anything before a new release

Yes, I've confirmed that the xarray test suite passes with the latest updates. I'm looking forward to eventually being able to remove some of the tolerance logic we have in our cftime round-tripping tests (we'll still support older versions of cftime for some time).

@spencerkclark spencerkclark mentioned this pull request Jan 4, 2021
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spencerkclark @jswhit